Improve performance of getLinkedOps
#58
Conversation
index.js
Outdated
|
|
||
| if (link.equals ? !link.equals(op._id) : link !== op._id) { | ||
| linkedOps.splice(i, 1); | ||
| } |
There was a problem hiding this comment.
It's not guaranteed that the next op's id would be equal to link, so this code could produce incorrect results in some cases.
There was a problem hiding this comment.
Do you know which cases exactly? Can we add test coverage?
There was a problem hiding this comment.
Yes, when the database returns 3 or more operations with the same version. It's extremely unlikely but possible. Operations with duplicate versions might be created when a ShareDB server stores an op but fails to store a corresponding snapshot, eg it crashes.
Actually, when I look at it again I think there are more issues here... i is an index in ops but you use it to remove items from linkedOps. Every time you remove from linkedOps, all following ops are re-indexed, so the next removal would be at a wrong index.
alecgibson
force-pushed
the
linked-op-performance-boost
branch
from
cae2d8d to
9a459eb
Compare
|
@gkubisa I've done as you suggested, and just replaced with |
Running `getOps` to fetch a large number of operations is not very performant. Anecdotally, performing this operation on a local development machine with a document comprising of ~200,000 operations takes ~20s. The largest slow-down appears to come from the `getLinkedOps` method, and in particular the use of `Array.unshift`. This change makes a very simple change that `push`es to the array instead of `unshift`, and then `reverse`s it at the end. The anecdotal operation time using this modified function is now ~2s, which is an improvement of an order of magnitude.
alecgibson
force-pushed
the
linked-op-performance-boost
branch
from
9a459eb to
66f6533
Compare
|
Looks good to me. Thanks @alecgibson 👍 |
|
@gkubisa do you know if anyone is still maintaining this repo? Or am I just going to have to fork? |
|
I maintain https://github.com/teamwork/sharedb-mongo and will merge this PR today. PRs should also start getting merged into this repo soon, hopefully after the next review meeting on Wednesday. |
|
@gkubisa tangentially related - do you know if there's anything we can do about how flaky these tests are?! I'm assuming I need a green build to merge? |
|
The build is ok at https://github.com/teamwork/sharedb-mongo thanks to @dcharbonnier: Hopefully those changes will make it into the official repos soon. |
|
@alecgibson I've just merged this branch into my fork and published to npm: https://www.npmjs.com/package/@teamwork/sharedb-mongo |
|
@alecgibson You can get the tests passing by updating sharedb-mingo-memory to ^1.0.1. See c147fc2. |
|
@gkubisa I've bumped the version, but there's possibly two more flaky tests? https://travis-ci.org/share/sharedb-mongo/jobs/405801814 |
|
Ok, that's for a very old version of node and MongoDB. I'd just update the .travis.yml file to test the supported node versions and fewer MongoDB versions like here: https://github.com/share/sharedb-mongo/pull/64/files#diff-354f30a63fb0907d4ad57269548329e3. @ericyhwang this PR #64 updates all modules and tested node and MongoDB versions. Could you review and merge it when you have a moment, please? |
alecgibson
force-pushed
the
linked-op-performance-boost
branch
from
f5ff21e to
66f6533
Compare
There was a problem hiding this comment.
Now that all the test flakiness PRs have been merged, this is good to go. Thanks again for diagnosing the issue and fixing it!
I tested this with the latest sharedb-mongo master, and the tests all pass, so I'm going to go ahead and merge+publish to not keep the PR waiting around any longer.
|
Published as [email protected] |
Running
getOpsto fetch a large number of operations is not veryperformant.
Anecdotally, performing this operation on a local development machine
with a document comprising of ~200,000 operations takes ~20s.
The largest slow-down appears to come from the
getLinkedOpsmethod,and in particular the use of
Array.unshift.This change makes a very simple change that
pushes to the arrayinstead of
unshift, and thenreverses it at the end.The anecdotal operation time using this modified function is now ~2s,
which is an improvement of an order of magnitude.